fix(deploy): handle SAS9 execution error gracefully - #1065

Hermes Agent Code Review

Verdict: Request changes — the core feature is effectively unreachable on the SAS9 path due to branch ordering; merge conflicts must also be resolved before merge.

ℹ️ Pre-review gate: GitHub reports this PR as mergeable: false / mergeable_state: dirty (the branch is far behind main). No check runs have run on the head SHA. I proceeded with a code review since the change is small and self-contained, but merge conflicts should be resolved before merging.

Summary

The PR adds a JobExecutionError branch to the SAS9 deploy catch handler so that, when a stored-process execution fails with errors, the SAS log is written to the usual .log file instead of being dropped. The intent (issue #1063) is sound and the approach (import JobExecutionError from the adapter, write err.result to the log path, then re-throw) is the right shape. However, the branch ordering prevents the new branch from being reached in the exact scenario the PR targets.

Correctness

• 🔴 Branch ordering defeats the feature (primary issue). The adapter's parseError (@sasjs/adapter RequestClient.ts) constructs JobExecutionError with errorCode: 404 for both the "stored process not found" case and the "Stored Process Error / This request completed with errors" case (the latter carries the valuable SAS log in result). Because the catch handler checks err.errorCode === 404 first, every JobExecutionError produced by the SAS9 execution path — including the stored-process-error case this PR is meant to handle — is routed to displaySasjsRunnerError(username) and never reaches the else if (err instanceof JobExecutionError) log-saving branch. In other words, the new log-saving code is effectively dead for the SAS9 path.
  • Fix: check err instanceof JobExecutionError before the generic 404 check, or distinguish the "runner missing" 404 (empty result) from the "stored process error" 404 (non-empty result). For example:

    .catch(async (err) => {
      if (err instanceof JobExecutionError) {
        if (err.result) {
          // stored process completed with errors — save the log
          process.logger?.error('Deployment completed with errors.')
          const errorLogPath = path.join(
            logFilePath || process.cwd(),
            `${path.basename(deployScript).replace('.sas', '')}.log`
          )
          await createFile(errorLogPath, err.result)
          process.logger?.info(`Error log is available at ${errorLogPath}`)
          throw new Error('Deployment completed with errors.')
        } else {
          // runner not found
          displaySasjsRunnerError(username)
        }
      } else {
        process.logger?.error(formatErrorString(err))
      }
    })

• 🟡 throw new Error() has no message. The empty-message Error makes the propagated failure opaque to callers and logs. Consider throw new Error('Deployment completed with errors. See log for details.') so the failure reason is preserved up the stack.
• 🟡 Regression in the 404 branch: error detail no longer logged. Previously formatErrorString(err) was logged for all errors before the 404 check; now the 404 branch only calls displaySasjsRunnerError and logs nothing about the underlying error. If the 404 is not the "runner missing" case (e.g. a JobExecutionError with errorCode: 404 and a real message), the diagnostic detail is lost. This is mitigated if the branch ordering is fixed as above.

Consistency

• 🟡 Diverges from the sibling run command. src/commands/run/run.ts handles the same SAS9 execution error by checking err.payload?.log (a different error shape) and throws an ErrorResponse. This PR uses err instanceof JobExecutionError / err.result. The two code paths now detect SAS9 execution failures differently. Worth confirming which error shape executeScriptSAS9 actually rejects with in current adapter versions (the packed @sasjs/adapter@3.0.0 types show JobExecutionError with result: string, and parseError is the throw path for SAS9 responses), and aligning both commands so future maintenance doesn't diverge further.

Tests

• 🟡 No test coverage for the new branch. There are no deploy specs exercising the JobExecutionError path (the deploy/spec/*.spec.ts files cover command wiring / getDeployScripts / cbd, not the SAS9 error handler). Since this is exactly the kind of branch-ordering bug that a unit test would catch ("given a JobExecutionError with errorCode: 404 and a non-empty result, the log file is written"), a regression test is strongly recommended. Mock sasjs.executeScriptSAS9 to reject with a JobExecutionError(404, 'This request completed with errors.', '<log>') and assert the .log file is created.

Style / Minor

• 🟢 executionResult ?? '' on line 278 is a good defensive touch for the success-path log write.
• The else { process.logger?.error('Unable to create log file.') } on line 291 is now reached when logFilePath is falsy or when execution failed but didn't throw (e.g. the 404 branch). The message is slightly misleading for the latter; acceptable but could be more specific.

Merge readiness

• Resolve merge conflicts against main (branch is significantly behind).
• Add a unit test for the JobExecutionError log-saving path (this would have caught the ordering bug).
• Fix the branch ordering so the feature actually takes effect on SAS9.

Reviewed by Hermes Agent (GitHub App)